Update cohort filtering in ParticipantsDeclarationsQuery #4794
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Review app deployed to https://cpd-ecf-review-4794-web.test.teacherservices.cloud |
cwrw
reviewed
May 16, 2024
| return ParticipantDeclaration.none if npq_lead_provider.blank? | ||
|
|
||
| with_joins(scope).where(participant_profile: { type: "ParticipantProfile::NPQ", schedule: { cohorts: { start_year: cohort_years } } }) | ||
| scope = with_joins(ParticipantDeclaration.for_lead_provider(cpd_lead_provider)) |
There was a problem hiding this comment.
Im wondering if we need the with_joins if there is no cohort filter? also worth revisiting those joins as we might no longer need some of them since cohort is on declaration now
ethax-ross
force-pushed
the
3056-filter-declarations-by-cohort
branch
from
c6807d0 to
fa7cec3
Compare
ethax-ross
force-pushed
the
3056-filter-declarations-by-cohort
branch
from
fa7cec3 to
397ed9d
Compare
ethax-ross
force-pushed
the
3056-filter-declarations-by-cohort
branch
from
34f6aec to
30e3704
Compare
ethax-ross
force-pushed
the
3056-filter-declarations-by-cohort
branch
from
30e3704 to
ab6ad4e
Compare
cwrw
reviewed
May 30, 2024
ethax-ross
force-pushed
the
3056-filter-declarations-by-cohort
branch
2 times, most recently
from
7d6950b to
9331367
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
cwrw
approved these changes
Jun 4, 2024
ethax-ross
force-pushed
the
3056-filter-declarations-by-cohort
branch
from
63e1bb6 to
1366044
Compare
Previously, we would filter using the induction records -> cohort for ECF and profile -> schedule -> cohort for NPQ participants. Now that we have a `cohort` directly on the `ParticipantDeclaration`, we can simplify the filtering logic.
As we filter by declaration -> cohort we no longer need to join on other cohorts. Move join on induction records to previous declarations scope as its only used there.
I wanted to do this in the factory originally, but it creates a cyclic dependency on the `participant_profile -> cohort` so we need to define it explicitly for now.
ethax-ross
force-pushed
the
3056-filter-declarations-by-cohort
branch
from
1366044 to
c57640f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
Jira-3059
Previously, we would filter using the
induction records -> cohortfor ECF andprofile -> schedule -> cohortfor NPQ participants. Now that we have acohortdirectly on theParticipantDeclaration, we can simplify the filtering logic.Changes proposed in this pull request
Guidance to review
I compared filtering against the snapshot DB using
mainand this branch and added a diff of where the API endpoint would return differently for a few providers/cohorts. The vast majority that differ are eithervoided,clawedbackorawaiting_clawbackand for the most part the stampedcohortmatches thefirst statement -> cohortbut theparticipant profile -> schedule -> cohortorlatest induction record -> cohortcauses it to return differently in the current declarations API.